Conversation
andrestejerina97
force-pushed
the
feature/add-badge-formatters
branch
from
b39cb5e to
e147182
Compare
andrestejerina97
force-pushed
the
feature/add-badge-formatters
branch
3 times, most recently
from
58b90cf to
bc5ed07
Compare
|
@andrestejerina97 |
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
LGTM Approved.
@martinquiroga-exo I agree that they're virtually identical and it seems wasteful/redundant in a way but it's also a set pattern. It's a good evaluation but not worth rejecting the PR - i.e. your nit vs rejection. Good call. The other PR also was solving for another problem - same entity type with different message formats. This one is the inverse. If we want to address it later we'd need to do so across the board.
|
|
||
| $summit = Mockery::mock(\models\summit\Summit::class); | ||
| $summit->shouldReceive('getName')->andReturn(self::SUMMIT_NAME); | ||
| $mock->shouldReceive('getSummit')->andReturn($summit); |
There was a problem hiding this comment.
@andrestejerina97 no test covers the getSummit() returns null path. The mock always returns a summit object. Add a test case where getSummit() returns null and verify the message contains 'Unknown Summit'.
this should be reflected on all tests
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@andrestejerina97 please review
andrestejerina97
force-pushed
the
feature/add-badge-formatters
branch
from
0821138 to
7689acb
Compare
andrestejerina97
force-pushed
the
feature/add-badge-formatters
branch
from
7689acb to
da94a17
Compare
|
Ready for review |
4 participants
ref: https://app.clickup.com/t/86b8emkur